fix: conditions reset does not work if the service is down at the triggering time #1585

juliev0 · 2022-02-02T17:45:50Z

This fix uses the cron syntax specified in the Condition Reset to determine the last time the trigger should have fired prior to application start up. If any messages arrive with a timestamp prior to that time, they are acknowledged.

whynowy

Great work!

common/cronutil_test.go

eventbus/driver/nats.go

juliev0 · 2022-02-03T01:52:37Z

Here's a description of how I tested this:

created a Sensor with a dependency of A && B && C and a Condition Reset and tested the following cases:
1a. per the bug, I scaled the deployment down to 0, waited for the CR time to pass, then scaled it back up to 1 to verify that the reset occurred for all dependencies
1b. I repeated the same test but not during the CR time to verify that no reset would occur
1c. standard Condition Reset logic: cron fired and reset occurred for all dependencies (to verify I didn't break this)
1d. standard logic: all 3 dependencies were sent (to verify I didn't break this)
also, to verify I didn't break anything I tried creating a Sensor with just one dependency and triggered it

whynowy · 2022-02-03T16:47:20Z

eventbus/driver/nats.go

 	if mh.isCleanedUp() {
-		mh.setLastResetTime(0)
+		mh.resetTimeout = 0


This should be protected by a lock, e.g. we could utilize setLastResetTime().

okay, fixed this

whynowy · 2022-02-03T16:47:37Z

eventbus/driver/nats.go

@@ -451,7 +459,7 @@ func (mh *eventSourceMessageHolder) resetAll() {
 	for k := range mh.parameters {
 		mh.parameters[k] = false
 	}
-	mh.setLastResetTime(0)
+	mh.resetTimeout = 0


Signed-off-by: Julie Vogelman <[email protected]>

…s time it should have fired Signed-off-by: Julie Vogelman <[email protected]>

…ventSourceMessageHolder.lastResetTime for performing this logic Signed-off-by: Julie Vogelman <[email protected]>

Signed-off-by: Julie Vogelman <[email protected]>

… and causes issues in this scenario: 'dependency=(A && B && C); trigger A and B, scale deployment to 0 before condition reset time; scale back to 1 after'; result: A gets reset, then sets lastResetTime to 0 because it's unknown that B still exists and B never gets acked Signed-off-by: Julie Vogelman <[email protected]>

Signed-off-by: Julie Vogelman <[email protected]>

…oj#1569) Bumps [github.com/nsqio/go-nsq](https://github.com/nsqio/go-nsq) from 1.0.8 to 1.1.0. - [Release notes](https://github.com/nsqio/go-nsq/releases) - [Changelog](https://github.com/nsqio/go-nsq/blob/master/ChangeLog.md) - [Commits](nsqio/go-nsq@v1.0.8...v1.1.0) --- updated-dependencies: - dependency-name: github.com/nsqio/go-nsq dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

…goproj#1559) Bumps [github.com/nats-io/stan.go](https://github.com/nats-io/stan.go) from 0.6.0 to 0.10.2. - [Release notes](https://github.com/nats-io/stan.go/releases) - [Commits](nats-io/stan.go@v0.6.0...v0.10.2) --- updated-dependencies: - dependency-name: github.com/nats-io/stan.go dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

Bumps [go.uber.org/zap](https://github.com/uber-go/zap) from 1.19.0 to 1.20.0. - [Release notes](https://github.com/uber-go/zap/releases) - [Changelog](https://github.com/uber-go/zap/blob/master/CHANGELOG.md) - [Commits](uber-go/zap@v1.19.0...v1.20.0) --- updated-dependencies: - dependency-name: go.uber.org/zap dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

This commit updates golangci-lint and removes the interfacer check because it was marked as deprecated and will be removed. See golangci/golangci-lint#541 for more information. Signed-off-by: William Van Hevelingen <[email protected]> Signed-off-by: Julie Vogelman <[email protected]>

…oproj#1579) Signed-off-by: William Van Hevelingen <[email protected]> Signed-off-by: Julie Vogelman <[email protected]>

…rgoproj#1574) Bumps [github.com/go-resty/resty/v2](https://github.com/go-resty/resty) from 2.3.0 to 2.7.0. - [Release notes](https://github.com/go-resty/resty/releases) - [Commits](go-resty/resty@v2.3.0...v2.7.0) --- updated-dependencies: - dependency-name: github.com/go-resty/resty/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

Signed-off-by: Julie Vogelman <[email protected]>

…goproj#1580) Bumps [cloud.google.com/go/compute](https://github.com/googleapis/google-cloud-go) from 0.1.0 to 1.1.0. - [Release notes](https://github.com/googleapis/google-cloud-go/releases) - [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md) - [Commits](googleapis/google-cloud-go@v0.1.0...dlp/v1.1.0) --- updated-dependencies: - dependency-name: cloud.google.com/go/compute dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

…roj#1575) Bumps [github.com/spf13/viper](https://github.com/spf13/viper) from 1.10.0 to 1.10.1. - [Release notes](https://github.com/spf13/viper/releases) - [Commits](spf13/viper@v1.10.0...v1.10.1) --- updated-dependencies: - dependency-name: github.com/spf13/viper dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

…ed for the failsafe >60 seconds condition Signed-off-by: Julie Vogelman <[email protected]>

Signed-off-by: Julie Vogelman <[email protected]>

…oj#1569) Bumps [github.com/nsqio/go-nsq](https://github.com/nsqio/go-nsq) from 1.0.8 to 1.1.0. - [Release notes](https://github.com/nsqio/go-nsq/releases) - [Changelog](https://github.com/nsqio/go-nsq/blob/master/ChangeLog.md) - [Commits](nsqio/go-nsq@v1.0.8...v1.1.0) --- updated-dependencies: - dependency-name: github.com/nsqio/go-nsq dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

…goproj#1559) Bumps [github.com/nats-io/stan.go](https://github.com/nats-io/stan.go) from 0.6.0 to 0.10.2. - [Release notes](https://github.com/nats-io/stan.go/releases) - [Commits](nats-io/stan.go@v0.6.0...v0.10.2) --- updated-dependencies: - dependency-name: github.com/nats-io/stan.go dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

…goproj#1584) Bumps [github.com/Shopify/sarama](https://github.com/Shopify/sarama) from 1.26.1 to 1.31.1. - [Release notes](https://github.com/Shopify/sarama/releases) - [Changelog](https://github.com/Shopify/sarama/blob/main/CHANGELOG.md) - [Commits](IBM/sarama@v1.26.1...v1.31.1) --- updated-dependencies: - dependency-name: github.com/Shopify/sarama dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

…rgoproj#1586) Bumps [github.com/xanzy/go-gitlab](https://github.com/xanzy/go-gitlab) from 0.50.2 to 0.54.4. - [Release notes](https://github.com/xanzy/go-gitlab/releases) - [Changelog](https://github.com/xanzy/go-gitlab/blob/master/releases_test.go) - [Commits](xanzy/go-gitlab@v0.50.2...v0.54.4) --- updated-dependencies: - dependency-name: github.com/xanzy/go-gitlab dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Julie Vogelman <[email protected]>

Signed-off-by: Henrik Blixt <[email protected]> Signed-off-by: Julie Vogelman <[email protected]>

…e set asyncronously through ConditionReset code Signed-off-by: Julie Vogelman <[email protected]>

whynowy · 2022-02-03T19:47:11Z

eventbus/driver/nats.go

+		mh.lock.Lock() // since this can be called asyncronously as part of a ConditionReset, we neeed to lock this code
+		defer mh.lock.Unlock()
+		mh.lastResetTime = t
+	}


Can we share the same lock and simply call setLastResetTime(0) instead of setRestTimeout(0)? It should be safe to set lastResetTime to 0, right?

mh.lock.Lock() defer mh.lock.Unlock() mh.lastResetTime = t if t == 0 { mh.resetTime = 0 } else { mh.resetTime = t + 60 }

No, one of the changes I made as part of this whole thing is that I never set lastResetTime to 0. lastResetTime now always represents an actual time. This ended up being necessary in a certain scenario.

Certain scenario such as?

Say you have condition A && B && C,
Say A and B both occur
Say the pod goes down while the condition reset time occurs
So, we want to be able to ack both A and B when they arrive but we don't: this is because if A arrives, we call reset(), which ends up doing this:
if mh.isCleanedUp() { mh.setLastResetTime(0) }
That is because the pod went down and B is no longer stored in the eventSourceMessageHolder's memory.
Then when B arrives we fail to ack it, because we would skip the logic which checks lastResetTime.

Good Point!

whynowy

LGTM, great work!

…ggering time (argoproj#1585) * incorporating new struct TimeBasedReset to encapsulate reset state/spec Signed-off-by: Julie Vogelman <[email protected]> Signed-off-by: Aaron Weisberg <[email protected]>

…ggering time (argoproj#1585) * incorporating new struct TimeBasedReset to encapsulate reset state/spec Signed-off-by: Julie Vogelman <[email protected]>

juliev0 force-pushed the issue_1543 branch from fe01a04 to 85046e2 Compare February 2, 2022 17:46

whynowy reviewed Feb 2, 2022

View reviewed changes

common/cronutil_test.go Show resolved Hide resolved

eventbus/driver/nats.go Show resolved Hide resolved

eventbus/driver/nats.go Show resolved Hide resolved

whynowy changed the title ~~bug: Issue 1543: conditions reset does not work if the service is down at the triggering time~~ fix: conditions reset does not work if the service is down at the triggering time Feb 3, 2022

juliev0 force-pushed the issue_1543 branch from 09dd7ec to 8791325 Compare February 3, 2022 00:25

juliev0 marked this pull request as ready for review February 3, 2022 01:45

juliev0 mentioned this pull request Feb 3, 2022

conditions reset does not work if the service is down at the triggering time. #1543

Closed

whynowy reviewed Feb 3, 2022

View reviewed changes

juliev0 and others added 22 commits February 3, 2022 11:20

incorporating new struct TimeBasedReset to encapsulate reset state/spec

3e7d58b

Signed-off-by: Julie Vogelman <[email protected]>

add capability to take a cron specification and determine the previou…

4692763

…s time it should have fired Signed-off-by: Julie Vogelman <[email protected]>

removing the TimeBasedReset struct; instead taking advantage of the e…

462990f

…ventSourceMessageHolder.lastResetTime for performing this logic Signed-off-by: Julie Vogelman <[email protected]>

Parser can be passed into PrevCronTime()

2a6e3f0

Signed-off-by: Julie Vogelman <[email protected]>

fix log statement so it actually shows the reset time

ddd70e3

Signed-off-by: Julie Vogelman <[email protected]>

PrevCronTime returns error

710c187

Signed-off-by: Julie Vogelman <[email protected]>

clarify comment

a588e73

Signed-off-by: Julie Vogelman <[email protected]>

chore(deps): bump google.golang.org/grpc from v1.42.0 to v1.43.0 (arg…

7b96c12

…oproj#1579) Signed-off-by: William Van Hevelingen <[email protected]> Signed-off-by: Julie Vogelman <[email protected]>

fix: e2e test not recognizing .kube/config (argoproj#1581)

35dcb13

Signed-off-by: Julie Vogelman <[email protected]>

adding fullResetOccurrenceTime to keep track of when the reset occurr…

d65b35a

…ed for the failsafe >60 seconds condition Signed-off-by: Julie Vogelman <[email protected]>

comments

0b0282f

Signed-off-by: Julie Vogelman <[email protected]>

removing commented out code

48dbb5e

Signed-off-by: Julie Vogelman <[email protected]>

comment

2560cef

Signed-off-by: Julie Vogelman <[email protected]>

less kludgey way of doing fullResetTimeout()

eb36a38

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 and others added 7 commits February 3, 2022 11:20

cleaner way to handle the reset time

e51bb0c

Signed-off-by: Julie Vogelman <[email protected]>

chore:Updating security.md (argoproj#1588)

2563de3

Signed-off-by: Henrik Blixt <[email protected]> Signed-off-by: Julie Vogelman <[email protected]>

adding mutex for eventSourceMessageHolder.resetTimeout since it can b…

71f8eb4

…e set asyncronously through ConditionReset code Signed-off-by: Julie Vogelman <[email protected]>

juliev0 force-pushed the issue_1543 branch from 733d566 to 71f8eb4 Compare February 3, 2022 19:22

whynowy reviewed Feb 3, 2022

View reviewed changes

whynowy approved these changes Feb 3, 2022

View reviewed changes

whynowy merged commit 9606aed into argoproj:master Feb 3, 2022

juliev0 deleted the issue_1543 branch February 3, 2022 22:48

tooptoop4 mentioned this pull request Mar 1, 2022

Release 1.6.0 breaks AMQP event source and loses messages #1694

Closed

juliev0 mentioned this pull request Apr 4, 2022

REQUEST: New membership for juliev0 (Julie Vogelman) argoproj/argoproj#124

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: conditions reset does not work if the service is down at the triggering time #1585

fix: conditions reset does not work if the service is down at the triggering time #1585

juliev0 commented Feb 2, 2022

whynowy left a comment

juliev0 commented Feb 3, 2022 •

edited

Loading

whynowy Feb 3, 2022

juliev0 Feb 3, 2022

whynowy Feb 3, 2022

whynowy Feb 3, 2022

juliev0 Feb 3, 2022

whynowy Feb 3, 2022

juliev0 Feb 3, 2022

whynowy Feb 3, 2022

whynowy left a comment

fix: conditions reset does not work if the service is down at the triggering time #1585

fix: conditions reset does not work if the service is down at the triggering time #1585

Conversation

juliev0 commented Feb 2, 2022

whynowy left a comment

Choose a reason for hiding this comment

juliev0 commented Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whynowy left a comment

Choose a reason for hiding this comment

juliev0 commented Feb 3, 2022 •

edited

Loading